Repository navigation
Conversation
…ation mode StandaloneApplicationClusterConfigurationParserFactory.getOptions() did not register SAVEPOINT_CLAIM_MODE or SAVEPOINT_RESTORE_MODE. Because the parser runs with stopAtNonOption=true, an unrecognized --claimMode did not fail: it ended option parsing and the remainder was swallowed into getArgs(). The claim mode was therefore unreachable in application mode and fell back to NO_CLAIM. Register both options so that createSavepointRestoreSettings can read them. Behavior is unchanged for jobs that pass neither option. Generated-by: GitHub Copilot CLI 1.0.80
createSavepointRestoreSettings read the claim mode only inside the branch guarded by --fromSavepoint, so passing --claimMode without a savepoint path returned SavepointRestoreSettings.none() and discarded it. Application-mode JobManagers hit this on HA recovery, where the checkpoint comes from HA storage rather than from a command line path. Branch on option presence instead, and add SavepointRestoreSettings.forRecoveryClaimMode for a claim mode with no restore path. The parsed mode is null exactly when neither option was passed, so an explicit --claimMode NO_CLAIM stays distinguishable from no override at all. allowNonRestoredState stays null unless the flag was passed, so the new branch does not start writing execution.state-recovery.ignore-unclaimed-state where nothing was written before (FLINK-39673). SavepointRestoreSettings.fromConfiguration has the same savepoint-path gate, so the `flink run` path still drops a pathless claim mode. That asymmetry is pre-existing and left untouched here; application mode writes directly to the JobManager configuration and does not go through it. Generated-by: GitHub Copilot CLI 1.0.80
|
Can you remove the first section of the PR description. |
Generated-by: GitHub Copilot CLI 1.0.83-5 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
thanks for the catch, have removed! |
|
@spuru9 Thanks for the review! I’ve combined the tests into a parameterized test and removed the introductory section from the PR description. Could you take another look when you have time? Please let me know if there are any remaining concerns with the scope or approach. |
…reconstruction Generated-by: GitHub Copilot CLI 1.0.84-5 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Jackeyzhe
left a comment
There was a problem hiding this comment.
Thanks for addressing the pathless recovery-mode round trip. I re-reviewed f214294 and confirmed that the explicitly configured claim mode now survives CLI parsing, configuration reconstruction, and StreamGraph creation.
No further concerns from my side.
|
Hi @Zakelly and @ferenc-csaky, could you help review this PR #29109 when you get a chance? It fixes Given your work on claim-mode handling and the CLI/application entrypoints, I’d appreciate your feedback on the approach. @Jackeyzhe has approved the latest revision, the existing review comments are addressed, and Azure CI is green. Thanks! cc @weiqingy |
What is the purpose of the change
Flink silently discards an explicitly-passed
--claimMode(or its deprecated alias--restoreMode), so the recovery claim mode falls back toNO_CLAIMwith no warning.This fixes FLINK-40559, including
the shared reconstruction issue reported in
FLINK-40562.
There are three causes:
StandaloneApplicationClusterConfigurationParserFactory.getOptions()never registersSAVEPOINT_CLAIM_MODE/SAVEPOINT_RESTORE_MODE. The parser runs withstopAtNonOption = true, so an unrecognized--claimModedoes not fail — it endsoption parsing and the remainder is swallowed into
getArgs().CliFrontendParser.createSavepointRestoreSettings()reads the claim mode only insidethe branch guarded by
--fromSavepoint. Passing--claimModewithout a savepointpath returns
SavepointRestoreSettings.none()and drops it. Application-modeJobManagers hit this on HA recovery, where the checkpoint comes from HA storage
rather than from a command line path.
SavepointRestoreSettings.fromConfiguration()returnsnone()when no savepointpath is present, dropping an explicitly configured mode during reconstruction in
both application and session submission paths.
This is not about configuration being overwritten. Since
FLINK-39673 /
PR #28295, an unset claim
mode is no longer written to the
Configuration, so a value fromflink-conf.yamlsurvives. The remaining defects concern CLI parsing and reconstructing pathless settings
from that configuration.
Brief change log
StandaloneApplicationClusterConfigurationParserFactory— registerSAVEPOINT_CLAIM_MODEandSAVEPOINT_RESTORE_MODE.CliFrontendParser— branch on option presence rather than on the presence of asavepoint path; extract
parseRecoveryClaimMode(). The parsed mode is null exactlywhen neither option was passed, so an explicit
--claimMode NO_CLAIMstaysdistinguishable from no override at all.
SavepointRestoreSettings— addforRecoveryClaimMode(...)for a claim mode with norestore path, and extend
toString()to render that state (it previously printednone()). Preserve pathless modes infromConfiguration()usinggetOptional,including the deprecated
execution.savepoint-restore-modealias and canonical-keyprecedence. Absent path and absent mode still return
none(), including allow-onlyconfiguration.
allowNonRestoredStatestaysnullunless the flag was passed, so the new branch doesnot start writing
execution.state-recovery.ignore-unclaimed-statewhere nothing waswritten before (the "explicitly set" semantics from FLINK-39673).
Option registration, parser restructuring, and shared reconstruction are separate commits.
Application and session scope
ProgramOptions.applyToConfiguration()writes the parsed mode, andExecutionConfigAccessor.getSavepointRestoreSettings()reconstructs it throughSavepointRestoreSettings.fromConfiguration().Application mode also reconstructs these settings: after
StandaloneApplicationClusterEntryPoint.loadConfigurationFromClusterConfig()writesthem to the configuration,
StreamGraphGeneratorreads them back, andPipelineExecutorUtils.getStreamGraph()reads them throughExecutionConfigAccessorand sets them on the graph again. The shared fix preserves pathless modes through
both paths; the earlier session-only limitation was incorrect.
This does not retroactively override persisted HA graphs.
EmbeddedExecutorcanrecover an existing job ID without submitting the newly generated graph.
Verifying this change
This change added tests and can be verified as follows:
StandaloneApplicationClusterConfigurationParserFactoryTest—--claimModeand--restoreModeare picked up by the application-mode parser, i.e. the options areregistered.
CliFrontendRunTest— pathless CLI settings now pass throughProgramOptionsandExecutionConfigAccessor. Parameterized application-graph coverage asserts settingsbefore and after
PipelineExecutorUtils.getStreamGraph()for all three modes.SavepointRestoreSettingsTest— fresh-configuration round trips preserve explicitmodes and nullable/false/true allow settings; absent options and allow-only behavior
remain unchanged. Deprecated-alias and canonical-key precedence are covered.
StreamGraphGeneratorTest— parameterized pathless-mode propagation through theexisting graph-generator fixture.
The original parser defects were first reproduced as failing tests on unmodified
master(expected: CLAIM but was: NO_CLAIM) and confirmed independent. For theshared reconstruction follow-up, the settings suite failed 15 cases and the graph
propagation test failed all 3 mode cases before the production fix; both passed afterward.
Dependent client tests were skipped in the graph red run and passed in the final run.
Latest local validation (JDK 17):
109 tests passed (31 settings, 32 graph-generator, 33 CLI, 13 container); no failures,
errors or skips. Spotless and Checkstyle passed. No live-cluster failover scenario or
full
clean verifywas run for this follow-up.Does this pull request potentially affect one of the following parts:
@Public(Evolving): noserialVersionUIDunchanged)graph construction/submission only)
Checkpointing, Kubernetes/Yarn, ZooKeeper: yes — changes how an explicit claim
mode reaches the JobManager configuration and newly generated/submitted graphs.
With neither a path nor an explicit mode, behavior remains unchanged. Explicit
pathless modes are now honored instead of silently discarded.
Documentation
option that was silently ignored.
Was generative AI tooling used to co-author this PR?
Generated-by: GitHub Copilot CLI 1.0.80; follow-up: GitHub Copilot CLI 1.0.84-5